api: Accept unknown "emojiset" values in initial snapshot #1983

MritunjayTiwari14 · 2025-11-09T13:37:20Z

Fixes #1981.

chrisbobbe · 2025-11-10T19:18:51Z

Let's make a new enum value unknown instead of using google.

MritunjayTiwari14 · 2025-11-11T11:59:25Z

Thank you @chrisbobbe for the review, introduced a new enum value unknown to which all the unknow Emojiset are directed to.

chrisbobbe

Thanks! Small comments below.

chrisbobbe · 2025-11-11T21:15:54Z

test/api/model/initial_snapshot_test.dart

+        'presence_enabled': true,
+      });
+
+      // Verify unknown emojiset defaults to Emojiset.unknow


nit: Emojiset.unknown

chrisbobbe · 2025-11-11T21:22:11Z

test/api/model/initial_snapshot_test.dart

+      final settings = UserSettings.fromJson({
+        'twenty_four_hour_time': true,
+        'display_emoji_reaction_users': true,
+        'emojiset': unknownValue,
+        'presence_enabled': true,
+      });


This .fromJson call will fail and need to be debugged each time we add a field to UserSettings.

Can you add a userSettings helper function in test/example_data.dart, on the pattern of similar helpers (like streamMessage or initialSnapshot), then I think this can just be

Suggested change

final settings = UserSettings.fromJson({

'twenty_four_hour_time': true,

'display_emoji_reaction_users': true,

'emojiset': unknownValue,

'presence_enabled': true,

});

final json = eg.userSettings().toJson()..['emojiset'] = unknownValue;

final settings = UserSettings.fromJson(json);

MritunjayTiwari14 · 2025-11-12T06:25:34Z

Thank you @chrisbobbe for the Suggestion, have pushed the changes. PTAL.

chrisbobbe

Thanks! Just small comments below.

Also something I just noticed on this round: for the commit-message summary line, let's say

api: Accept unknown "emojiset" values in initial snapshot

chrisbobbe · 2025-11-12T19:12:51Z

test/example_data.dart

+  return UserSettings(
+    twentyFourHourTime: twentyFourHourTime ?? TwentyFourHourTimeMode.twentyFourHour,
+    displayEmojiReactionUsers: displayEmojiReactionUsers ?? true,
+    emojiset: emojiset ?? Emojiset.unknown ,


We should aim to make the defaults boring and representative of real data, and I think Emojiset.google would be better for that than Emojiset.unknown.

Also this helper can deduplicate some code in the initialSnapshot helper just below this.

Just below the userSettings function declaration, you can say

const _userSettings = userSettings;

then in initialSnapshot in choosing arguments for the InitialSnapshot constructor, you can say:

userSettings: userSettings ?? _userSettings(),

MritunjayTiwari14 · 2025-11-15T09:06:28Z

Thank you @chrisbobbe for noticing that, I've also changed the value of TwentyFourHourTimeMode.twentyFourHour to default boring i.e TwentyFourHourTimeMode.twelveHour.

chrisbobbe

Thanks! One nit below, and I'll mark this for Greg's review.

chrisbobbe · 2025-11-18T23:42:03Z

test/example_data.dart

+}
+
+const _userSettings = userSettings;


nit: remove blank line above const _userSettings = userSettings;, following the pattern of other helpers in this file

Fixes zulip#1981.

MritunjayTiwari14 · 2025-11-22T06:16:51Z

Thank you @chrisbobbe for the review, the new changes had been pushed. PTAL @gnprice.

gnprice

Thanks @MritunjayTiwari14 for taking care of this, and thanks @chrisbobbe for the previous reviews! A few comments below.

gnprice · 2025-11-25T03:23:16Z

lib/api/model/initial_snapshot.dart

+  @JsonKey(unknownEnumValue: Emojiset.unknown)
  Emojiset emojiset;


I looked at other references to Emojiset to see if we might need this elsewhere.

One is in events:

case UserSettingName.emojiset: return Emojiset.fromRawString(value as String);

It looks like the effect of that is that if the user's value for this setting gets changed to an unknown value, we'd still throw. Let's fix that so that it produces Emojiset.unknown, just like if the same unknown value is found in the initial snapshot.

gnprice · 2025-11-25T03:25:51Z

test/api/model/initial_snapshot_test.dart

+
+      // Verify unknown emojiset defaults to Emojiset.unknown
+      check(settings.emojiset).equals(Emojiset.unknown);


This comment doesn't add anything that isn't said equally clearly by the code. So best to leave it out:

Suggested change

// Verify unknown emojiset defaults to Emojiset.unknown

check(settings.emojiset).equals(Emojiset.unknown);

check(settings.emojiset).equals(Emojiset.unknown);

gnprice · 2025-11-25T03:28:18Z

test/api/model/initial_snapshot_test.dart

+  test('UserSettings.emojiset handles various unknown values', () {
+    final unknownValues = ['apple', 'microsoft', 'facebook', ''];
+    for (final unknownValue in unknownValues) {


After the first of these, there's no useful information expressed by checking the next two — it's hard to imagine what sort of possible bug could be introduced in the future that would hit when the value were "microsoft" but not when it's "apple". So simplify the story by leaving them out:

Suggested change

test('UserSettings.emojiset handles various unknown values', () {

final unknownValues = ['apple', 'microsoft', 'facebook', ''];

for (final unknownValue in unknownValues) {

test('UserSettings.emojiset handles various unknown values', () {

final unknownValues = ['apple', ''];

for (final unknownValue in unknownValues) {

(OTOH the empty string does add information, since that's an edge case where it's more conceivable that there could be a bug specifically affecting that case.)

MritunjayTiwari14 force-pushed the issue_1981 branch from df1e800 to 9382645 Compare November 11, 2025 11:58

chrisbobbe reviewed Nov 11, 2025

View reviewed changes

MritunjayTiwari14 force-pushed the issue_1981 branch 2 times, most recently from 1896109 to 59cd993 Compare November 12, 2025 06:21

chrisbobbe reviewed Nov 12, 2025

View reviewed changes

gnprice added the maintainer review PR ready for review by Zulip maintainers label Nov 13, 2025

MritunjayTiwari14 force-pushed the issue_1981 branch from 59cd993 to 7e79010 Compare November 15, 2025 09:02

MritunjayTiwari14 requested a review from chrisbobbe November 15, 2025 09:04

MritunjayTiwari14 changed the title ~~initial_snapshot: Accept unknown "emojiset" values~~ api: Accept unknown "emojiset" values in initial snapshot Nov 15, 2025

chrisbobbe reviewed Nov 18, 2025

View reviewed changes

chrisbobbe requested a review from gnprice November 18, 2025 23:43

chrisbobbe assigned gnprice Nov 18, 2025

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Nov 18, 2025

api: Accept unknown "emojiset" values in initial snapshot

7fc7cd8

Fixes zulip#1981.

MritunjayTiwari14 force-pushed the issue_1981 branch from 7e79010 to 7fc7cd8 Compare November 20, 2025 09:16

gnprice reviewed Nov 25, 2025

View reviewed changes

		@JsonKey(unknownEnumValue: Emojiset.unknown)
		Emojiset emojiset;


		// Verify unknown emojiset defaults to Emojiset.unknown
		check(settings.emojiset).equals(Emojiset.unknown);

api: Accept unknown "emojiset" values in initial snapshot #1983

Are you sure you want to change the base?

api: Accept unknown "emojiset" values in initial snapshot #1983

Conversation

MritunjayTiwari14 commented Nov 9, 2025

Uh oh!

chrisbobbe commented Nov 10, 2025

Uh oh!

MritunjayTiwari14 commented Nov 11, 2025

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MritunjayTiwari14 commented Nov 12, 2025

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MritunjayTiwari14 commented Nov 15, 2025

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MritunjayTiwari14 commented Nov 22, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants